Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update code.md #2718

Merged
merged 2 commits into from
Nov 10, 2023
Merged

Update code.md #2718

merged 2 commits into from
Nov 10, 2023

Conversation

tstastny
Copy link
Contributor

@tstastny tstastny commented Sep 1, 2023

One more exception to google style. @hamishwillee @bkueng

FYI - i can remove the part about "tab-ability" .. that was an insight from @bkueng :) but in general, i think it will be quite hard to get folks to switch to postfixed underscores.. and a lot of code to change.

One more exception to google style. @hamishwillee @bkueng 

FYI - i can remove the part about "tab-ability" .. that was an insight from @bkueng :) but in general, i think it will be quite hard to get folks to switch to postfixed underscores.. and a lot of code to change.
@tstastny tstastny self-assigned this Sep 1, 2023
Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable - was this broadly agreed and communicated with the dev team?

@tstastny
Copy link
Contributor Author

tstastny commented Sep 4, 2023

Came out of a discussion with @bkueng. Just threw it up so I didn't forget. We should discuss in Tuesday maintainer call before pulling trigger.

@hamishwillee
Copy link
Collaborator

Ok, keep me posted. FWIW I must be an old dog, because I don't see why you would want post underscores - the prefix more immediately tells you the variable is private.

@dagar
Copy link
Member

dagar commented Sep 5, 2023

Underscore prefix is a little controversial because there are some other rules that are a bit too close like in any scope underscore + uppercase is reserved, double underscore is reserved, and in a global namespace anything with underscore prefix could be reserved.

So even with all that I personally still have a mild preference for it, but let's just stop talking about the google style guide if we're going to carry all of these exceptions.

FYI this is why the state covariance matrix in EKF2, LPE, etc isn't _P, it will actually fail to compile on certain platforms.

@hamishwillee
Copy link
Collaborator

Appreciate the explanation.

stop talking about the google style guide if we're going to carry all of these exceptions.

I think you're saying "lets not make this change because we want to stick to the guide as closely as possible"? If so, this is supposed to be discussed in the maintainer call.

I don't care what the rules are, but I think we should lint to enforce them.

@hamishwillee
Copy link
Collaborator

@tstastny Did this get discussed/resolved in any way?

Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't agree more. I was close to suggesting this as well but didn't want to introduce any opinionated extras.

@tstastny
Copy link
Contributor Author

@tstastny Did this get discussed/resolved in any way?

shoot - i keep forgetting to bring it up in the dev calls. will put on the discussion list for next one.

@hamishwillee
Copy link
Collaborator

Don't forget @tstastny :-)

@hamishwillee
Copy link
Collaborator

@tstastny Looking for that comment in the dev call notes .... I guess you've forgotten that we'll both be at the PX4 developer day soon right and I can apply some menaces :-)

@hamishwillee
Copy link
Collaborator

@tstastny Grrr :-)

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-maintainers-call-november-07-2023/35031/1

@tstastny
Copy link
Contributor Author

tstastny commented Nov 7, 2023

From maintainer call: https://discuss.px4.io/t/px4-maintainers-call-november-07-2023/35031
@dagar please feel free to correct anything i got wrong here. again, I'm not terribly opinionated.. just want to lock it down so we don't have to think about it anymore, and have consistency.

  • Abandon attempts to square with Google Style
  • Update guide for what we are actually doing
    • e.g. some stragglers like file names (headers)
  • Get linting set up FIRST so that we can actually enforce automatically (@MaEtUgR started this, we will need to find time to revisit and get it up and running)

@hamishwillee ok for you? i can revise the style guide for the main things we actually have traditionally cared about and intend to lint.

@MaEtUgR
Copy link
Member

MaEtUgR commented Nov 7, 2023

started this, we will need to find time to revisit and get it up and running

Clang format (only spacing): PX4/PX4-Autopilot#19855
Clang tidy I still have to spend some time.

@hamishwillee
Copy link
Collaborator

@tstastny Thanks for the update.

Sounds sensible, in particular to get linting working first and have it be the canonical definition of our style guide. Once the PR is accepted would be great if you could update this guide and I can review.

As an aside, it doesn't appear to be "quite" Abandon attempts to square with Google Style since the clang setup is based on google. It is a good thing to be based as close to a documented style as possible.

FYI (and to you too @MaEtUgR ) we might also want to look at https://github.com/reviewdog/reviewdog . MDN use this tool to auto-lint changes made though github and add fixes as suggestions. It might not be useful for this, but I plan to integrate it into docs at some point.

Copy link

No flaws found

@MaEtUgR
Copy link
Member

MaEtUgR commented Nov 10, 2023

This clarification is more correct than before since almost all code has this style right now. We still need to enforce by linting but this change has no disadvantage.

@MaEtUgR MaEtUgR merged commit a33a4cd into main Nov 10, 2023
1 of 2 checks passed
@MaEtUgR MaEtUgR deleted the update-code-style-guidelines branch November 10, 2023 20:09
@hamishwillee
Copy link
Collaborator

Works for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants